fix: add support for idle connection monitoring (opt-in for now) #1171
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| // `connectionAcquired` event and will be complete before any future `connectionReleased` event could fire for | ||
| // the same connection. | ||
| monitors.remove(connection)?.let { monitor -> | ||
| 5.seconds |
| // Use `runBlocking` because this _must_ finish before OkHttp goes to use the connection | ||
| val cancelTime = measureTime { | ||
| runBlocking(context) { monitor.cancelAndJoin() } | ||
| } |
There was a problem hiding this comment.
Why is measuring the cancel time important here?
There was a problem hiding this comment.
The time spent waiting to cancel is blocking and delays the usage of the connection. Measuring that time and emitting it in a subsequent log message can help users tune their polling interval.
| while (coroutineContext.isActive) { | ||
| try { | ||
| logger.trace { "Polling socket for $conn" } | ||
| source.readByte() // Blocking read; will take up to READ_TIMEOUT_MS to complete |
There was a problem hiding this comment.
will take up to READ_TIMEOUT_MS to complete
question/clarification request: I don't see a READ_TIMEOUT_MS / any other timeout used here in the readByte call
There was a problem hiding this comment.
Yep, leftover from when the polling interval was a fixed constant instead of a configurable parameter. Will update.
| if (resetTimeout) { | ||
| logger.trace { "Attempting to reset soTimeout..." } | ||
| try { | ||
| conn.socket().soTimeout = oldTimeout |
There was a problem hiding this comment.
question: Is it possible for a monitored connection to be pulled and used by OkHttp before we reset the soTimeout?
There was a problem hiding this comment.
Sort of. We start monitoring when a connection is released after a call and we cancel monitoring when a connection is acquired in preparation for a call. At the point we get the connectionAcquired event, OkHttp has already identified the connection it will use from the pool and run some health checks on it. But as far as I can see (and as far as my testing has shown), OkHttp won't actually send/receive data from the socket until after this event is complete.
There was a problem hiding this comment.
Do we have measurements for how long those health checks last and how long we take to reset the soTimeout? We might have a lot of overhead or maybe not.
There was a problem hiding this comment.
Nvm this comment, I was just misunderstanding something
This comment has been minimized.
This comment has been minimized.
| source.readByte() // Blocking read; will take up to `pollInterval` time to complete | ||
| } catch (_: SocketTimeoutException) { | ||
| logger.trace { "Socket still alive for $conn" } | ||
| // Socket is still alive |
There was a problem hiding this comment.
Nit: This comment seems unnecessary (// Socket is still alive). Feel free to ignore though.
Affected ArtifactsSignificantly increased in size
Changed in size
|
Issue #
Resolves awslabs/aws-sdk-kotlin#1214
Description of changes
This PR adds a new opt-in idle connection monitor for OkHttp, configurable by the engine parameter
connectionIdlePollingInterval. This setting is opt-in for now to preserve existing behavior but will be switched to opt-out (or replaced) in a future minor release.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.